@joshheald I wonder what you think as well.

Short context: In order to get imageURLs for order items, I need to get products first. This one is a naive implementation for POS, which relies on making API calls for selected orders to get the images. Is it realistic to rely on GRBD product storage in the near future and only make API calls in case the product doesn't exist (for large catalogs)? Thanks!

GRDB storage will include image URLs (and other image properties), so yes...

But... we'll only populate the catalog for smaller stores which have the new variation API endpoint. So there will be other stores which don't get the image that way.

I'd be tempted to say that images are a nice-to-have here, and that trade off is OK, to the point of not fetching products/images for non-catalog stores.

However... are you sure you need to?

When I request /wc/v3/orders/, without specifying _fields, the response includes image details:

{
    "id": 2898,
    "parent_id": 0,
    "status": "completed",
    "currency": "USD",
    "version": "10.1.2",
    [...]
    ],
    "line_items": [
      {
        "id": 8601,
        "name": "A different beanie",
        "product_id": 1523,
        [...]
        "image": {
          "id": "103",
          "src": "https:\/\/jheverydaypay.mystagingwebsite.com\/wp-content\/uploads\/2023\/08\/beanie-2.jpg"
        },
        [...]
      }
    ],
 [...]

Now I see you're using those below. Why not map from the productID in the line_item instead of fetching the whole product?

When I request /wc/v3/orders/, without specifying _fields, the response includes image details:

Okay.. I missed it by looking at how it looks on the Woo app

Good that I asked, it will be a big simplification. 🙇

Should this be renamed to imageURLString or similar instead?

Well, its type is String, so maybe it's fine not appending that to the name?

Yeah, that's my point, the property seems to indicate that will hold an URL type when it's a string. What about imageSource? I might be over reading it, so feel free to ignore :D

👀 I'm guessing this was automatically generated on rake generate, but I'm not sure why. Perhaps some other PR missed it? Since builds without the import, we could remove it from this PR.

Yes, it was added by rake generate.

Since builds without the import, we could remove it from this PR.

Well.. it would re-appear with another rake generate so probably not much value in removing it here.

I just noticed that the test target won't run locally unless we add this import via rake generate

Undefined symbol: NetworkingCore.AlamofireNetwork.__allocating_init(credentials: NetworkingCore.Credentials?, selectedSite: Combine.AnyPublisher<NetworkingCore.JetpackSite?, Swift.Never>?, sessionManager: Alamofire.Session?) -> NetworkingCore.AlamofireNetwork

Must have sneaked through some other changes, still odd that CI has been passing.

Should we mark the function as async and remove the Task block ?

I want the selection to happen synchronously, and then later update to the selected order to happen asynchronously. So I think it's appropriate to keep this code in a task block. wdyt?

Oh sure, that sounds better than doing everything async 👍

Is this needed? It's only used in resolveImages, where we already perform the empty check.

Should we wrap this in a do/catch block and log/bubble-up if there's an error? Otherwise might fail silently from throwing on the remote.

I wasn't sure if there was value in even logging some network/server failure 🤔 Of course, we could add it to help out with troubleshooting some issues where users don't see photos for their order products.